Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factory sort tactic #252

Merged
merged 54 commits into from
Jul 22, 2021
Merged

Factory sort tactic #252

merged 54 commits into from
Jul 22, 2021

Conversation

gares
Copy link
Member

@gares gares commented Jun 15, 2021

This should make Factory.sort unnecessary and speed up compilation.

We use xpack to avoid conflicts.
This now requires LPCIC/coq-elpi#257
The status is the following:

  • Global HB.pack tactic which can be used for any structure (but no planB), works as pose X : S.type := HB.pack T F
  • Structure.xpack generated for any structure. Note that it's not a real qualification, it's a global notation; works as pose X := S.xpack T F
  • short(pack = "name")
  • logging to fix planB (may require patching coq.hb since the loc are right, but the patch is not line based, but rather char based)

Fix #253

@gares gares changed the title Factory sort tractic Factory sort tactic Jun 18, 2021
@gares gares force-pushed the factory_sort_tractic branch 2 times, most recently from bd2a342 to eef2a1e Compare June 18, 2021 13:17
@gares gares added this to the 1.2.0 milestone Jun 18, 2021
@gares gares force-pushed the factory_sort_tractic branch 4 times, most recently from 40e1fdb to be2e9a8 Compare June 22, 2021 16:21
@gares gares force-pushed the factory_sort_tractic branch from 45f33f8 to 171dd27 Compare June 29, 2021 14:43
@CohenCyril
Copy link
Member

@gares shall we merge?

@gares
Copy link
Member Author

gares commented Jul 1, 2021

We have to decide what to do with the code of eta and factory.sort, which is half commented

@gares
Copy link
Member Author

gares commented Jul 1, 2021

I would wait to talk about it live, maybe on Tuesday?

@gares gares force-pushed the factory_sort_tractic branch from ebf5846 to 7b659c6 Compare July 21, 2021 12:35
@gares
Copy link
Member Author

gares commented Jul 21, 2021

@CohenCyril this is green again. I could measure precisely the performances and:

  • "factory sort" does not add anything per se
  • "factory sort tac" has exactly the same performance

This Pr math-comp/math-comp#770 contains a cleanup commit and another one where I use HB.pack behind the scenes. To me, it looks like an improvement, especially since one does not need to retrofit some inheritance by sneaking a structure projection somewhere. After the last commit you can write HB.pack Key Tsomething Factory, which is quite natural, eg pose Kfield := FieldType K Kring KfieldMixin. (Kring is a structure, but HB.pack projects it to its class which is a factory, so you can use K as the real key, and after it all the stuff).

So to me it seems the factory sort via CS is superseded, since there is no performance loss.

The only mixed feeling left is that FooType becomes a notation (a grammar rule) and not a simple constant.
IMO, it is worth using, systematically, pose Kfield : fieldType := HB.pack K Kring KfieldMixin which requires no notation, works with planB and is only a few more chars per line. Then #[pack(short="xxx")] would enable the notation xxx, which is otherwise skipped. Finally, HB.pack would look less mysterious (it is clearly some HB code you can look up in the doc and not a magic constant with variable arity).

Once we merge this PR I can start playing with strategy again. Do you agree with the plan above (use HB.pack directly).

@CohenCyril
Copy link
Member

Yes, I agree.
How does HB.pack work with plan B BTW?

@gares
Copy link
Member Author

gares commented Jul 22, 2021

HB.pack is an exported tactic. Elpi Export tac is the same thing of tac-in-terms, it adds a grammar rule. But this time the grammar rule is just one and mentions HB, so it is more legit that it is global (it is governed by Import structures, not by Import GRing.Foo).

Being a grammar rule it receives the right loc, so in planB you get it replaced by Foo.Pack T (Foo.Class T M1 .. MN)

@CohenCyril
Copy link
Member

HB.pack is an exported tactic. Elpi Export tac is the same thing of tac-in-terms, it adds a grammar rule. But this time the grammar rule is just one and mentions HB, so it is more legit that it is global (it is governed by Import structures, not by Import GRing.Foo).

Being a grammar rule it receives the right loc, so in planB you get it replaced by Foo.Pack T (Foo.Class T M1 .. MN)

OK, so this means planB stops usercode from packing inside code... While before they "just" could not add a new structure...
Isn't this too much of a limitation for plan B to make sense?

@gares
Copy link
Member Author

gares commented Jul 22, 2021

OK, so this means planB stops usercode from packing inside code... While before they "just" could not add a new structure...

Can you exaplian better? I don't follow.

@CohenCyril
Copy link
Member

Can you exaplian better? I don't follow.

Yes sorry. As I understand it the purpose of HB plan B is to reassure HB users developing a library such as mathcomp, that they do not have to depend on elpi, they can translate everything and ship the code without elpi. The cost is the unability to add new structures, but the rest is supposed to work just fine. Now, if building an instance in the middle of the proof requires HB, then it impacts users as well... One could argue that without HB.instance it's already difficult to use HB though... so in the end I don't know, let's say it's fine like you propose.

@gares gares merged commit be26d54 into master Jul 22, 2021
@gares gares deleted the factory_sort_tractic branch October 8, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the error about NFI a warning for some time
2 participants